Skip to content

feat(seer): Allow bulk-editing Code Review triggers#113116

Open
ryan953 wants to merge 4 commits intomasterfrom
ryan953/seer-code-review-bulk-triggers
Open

feat(seer): Allow bulk-editing Code Review triggers#113116
ryan953 wants to merge 4 commits intomasterfrom
ryan953/seer-code-review-bulk-triggers

Conversation

@ryan953
Copy link
Copy Markdown
Member

@ryan953 ryan953 commented Apr 15, 2026

SCR-20260415-mnma

@ryan953 ryan953 requested review from a team as code owners April 15, 2026 21:03
@ryan953 ryan953 requested a review from billyvg April 15, 2026 21:03
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 15, 2026
for (const repo of selectedRepos) {
if (repo.settings?.codeReviewTriggers?.length === 0) {
repoIdsWithZeroTriggers.push(repo.id);
} else if (!repo.settings?.codeReviewTriggers.includes(added)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A missing optional chaining operator (?.) on codeReviewTriggers will cause a TypeError when bulk-editing a repository that has null settings.
Severity: CRITICAL

Suggested Fix

Add an optional chaining operator before the .includes call on line 168. The line should be changed to } else if (!repo.settings?.codeReviewTriggers?.includes(added)) { to prevent a TypeError when repo.settings is null.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
static/gsApp/views/seerAutomation/components/repoTable/seerRepoTableHeader.tsx#L168

Potential issue: In the `handleBulkTriggers` function, the code attempts to access
`repo.settings?.codeReviewTriggers.includes(added)`. However, it is missing an optional
chaining operator (`?.`) before the `.includes` call. According to the type definitions,
`repo.settings` can be `null`. If a user performs a bulk edit on a repository where
`settings` is `null`, `repo.settings?.codeReviewTriggers` will evaluate to `undefined`,
and the subsequent attempt to call `.includes` on `undefined` will cause a `TypeError`,
crashing the component.

Did we get this right? 👍 / 👎 to inform future reviews.

Comment on lines +110 to +116
const someOnReadyForReview = selectedRepos.every(repo =>
repo?.settings?.codeReviewTriggers?.includes('on_ready_for_review')
);
const someOnNewCommit = selectedRepos.every(repo =>
repo?.settings?.codeReviewTriggers?.includes('on_new_commit')
);
return [
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The currentTriggersValue calculation using .every() on an empty selectedRepos array results in a "vacuous truth", incorrectly populating the bulk edit trigger state during transient renders.
Severity: LOW

Suggested Fix

Add a guard at the beginning of the useMemo hook for currentTriggersValue. If selectedRepos.length === 0, return an empty array [] to prevent the .every() logic from executing on an empty array and causing a vacuous truth.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
static/gsApp/views/seerAutomation/components/repoTable/seerRepoTableHeader.tsx#L110-L116

Potential issue: The `currentTriggersValue` is calculated using
`Array.prototype.every()` on the `selectedRepos` array. When `selectedRepos` is empty,
`.every()` returns `true` due to vacuous truth. This can occur in a transient state
during a query change where selections exist (`isAnySelected` is true) but the filtered
repository list (`selectedRepos`) is temporarily empty. This results in the UI
incorrectly showing all bulk edit triggers as selected. Interacting with this incorrect
state can cause incorrect bulk update requests to be sent to the server.

Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 85e0a18. Configure here.

);
const someOnNewCommit = selectedRepos.every(repo =>
repo?.settings?.codeReviewTriggers?.includes('on_new_commit')
);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variables named some... actually use every() semantics

Low Severity

The variables someOnReadyForReview and someOnNewCommit are named with the prefix some but actually use .every(), which checks if ALL selected repos have the trigger. The naming directly contradicts the semantics — .some() returns true if any element matches, while .every() returns true only if all match. The every logic is correct for this feature (only show a trigger as selected when all repos have it), but the misleading names could cause a future developer to misunderstand the behavior.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 85e0a18. Configure here.

enabledCodeReview?: never;
}
| {
codeReviewTriggers: string[];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second union variant missing CodeReviewTrigger type update

Low Severity

The codeReviewTriggers field in the second union variant of RepositorySettings (line 19) still uses string[], while the third variant (line 24) was updated to CodeReviewTrigger[]. The handleBulkTriggers calls that pass only codeReviewTriggers without enabledCodeReview match the second variant, so they bypass the stricter CodeReviewTrigger type checking that the commit intended to introduce.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 85e0a18. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant